Additional action logging#7270
Conversation
labkey-adam
left a comment
labkey-adam
left a comment
There was a problem hiding this comment.
This looks good, other than my comment about logging the user info.
As we discussed in standup, this approach will ensure minimal logging but highlight failures. In the future, I could see adding trace logging for the cases that pass, so one can see the entire permission checking process, if trace is available. Plus details about what permission(s) was/were missing, etc.
| Container c = context.getContainer(); | ||
| User user = context.getUser(); | ||
| Class<? extends Controller> actionClass = getClass(); | ||
|
|
There was a problem hiding this comment.
Shouldn't we unconditionally log the user and (if impersonated) the impersonating user? I know that'll increase the logs, but how else will Alex zero in on the users that are causing problems?
| if (shouldThrow) | ||
| throw new ForbiddenProjectException("You are not allowed to access this folder while impersonating within a different project."); | ||
| { | ||
| String msg = "You are not allowed to access this folder while impersonating within a different project."; |
There was a problem hiding this comment.
This causes no harm, but I think I improved what we return to API callers to include "forbidden project" exception messages. Of course, one has to look at those messages...
|
I did some manual testing on this with no problems. Agree that the user logging makes this a bit verbose. I lean toward merging this and adjusting as we learn more from clients. Potential future refactor: add user and detailed message to UnauthorizedException. Catch UnauthorizedException in caller and log.debug() there. |
2 participants
Rationale
This addresses #720 by adding additional DEBUG logging.
Changes